@bors try @rust-timer queue

💥 Test timed out after 21600s

Retrying CI since #t-infra > CI keeps failing because of npm error seems to be fixed. Not sure what's up with the bors2 timeout but let's try that again too I guess?

@bors try

☀️ Try build successful (CI)
Build commit: 03a6515 (03a6515b1ba7e8b2d5a4165ce136197e7d22feb3, parent: ceb7df7e6f17c92c7d49f7e4f02df0e68bc9b38b)

Finished benchmarking commit (03a6515): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @rustbot label: +perf-regression-triaged. If not, please fix the regressions and do another perf run. If its results are neutral or positive, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

Max RSS (memory usage)

Cycles

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 475.105s -> 475.535s (0.09%)
Artifact size: 390.80 MiB -> 390.76 MiB (-0.01%)

So that's where the deep-vector regression is from! To be honest, I'm not confident I know what's going on. I tried profiling locally to get a better idea of where the instruction count difference is, and it looks like the biggest diff by far is malloc_default with 11,972,450 more instructions(?), and the second biggest is emap_try_acquire_edata_neighbor with 2,270,678 more instructions(?). So apparently it's spending more instructions in the allocator? Maybe changing when the ScopeTree is built happens to be worse for deep-vector, somehow? For pathological cases like deep-vector, the scope tree's parent scope map is very large and it's not initialized with a capacity or anything so it probably needs a lot of reallocations; being later in compilation is probably bad for that? This also slightly changed the sizes of ScopeTree and TypeckResults, but that seems less likely to be it.

I do have an couple ideas that could improve perf both here and for realistic code, but they're not pretty. Reifying the scope hierarchy isn't really necessary, and there's a few ways to avoid it; e.g.:

• Currently, the ScopeTree's parent scope map is effectively a copy of the HIR for a typeck root but allocated in a hash map rather than an arena and more convenient for doing upwards traversals on. But we could do upwards traversals directly on the HIR. Upwards traversals on the HIR are unpleasant though, so I don't know whether it would be worth it.
• We could avoid both the upwards traversals and building a parent map if we do scope resolution in the same pass as building the THIR. We'd just be doing more bookkeeping on the way down. This would probably have the best perf, but it's not good for separating concerns. It's also a bit inconvenient for a few shadowing-related diagnostics/lints, which simpler to write when there's an easy way to inspect the scope hierarchy; getting rid of upwards traversals means they'd have to implement their own most likely.

Another option: we remove the TempLifetime from thir::Expr. The MIR Builder has access to the ScopeTree for getting variables' scopes, so why not get temporary scopes that way too? There may be a reason for this, but I'm not aware of it if so. Making thir::Expr smaller and doing less work per expression node would probably help. I may try this.